BUG: Propagate Python exceptions from c_is_list_like (#33721) #33723

shwina · 2020-04-22T13:45:47Z

closes BUG: Exceptions not propagated from c_is_list_like in lib.pyx #33721
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

WillAyd · 2020-04-22T15:10:26Z

pandas/_libs/lib.pxd

@@ -1 +1 @@
-cdef bint c_is_list_like(object, bint)
+cdef bint c_is_list_like(object, bint) except -1


Hmm don't think a bint can ever return -1. The only thing I think you could except here is NULL but would really have to refactor to do that

Is there a particular issue you are trying to solve?

Thanks for reviewing.

The ignored exception led to a segfault in https://github.com/rapidsai/cudf that I'm unable to reproduce at the moment since we've since refactored our code a bit -- apologies.

However, we can demonstrate the issue by raising artificially within c_is_list_like -- I believe it's valid to assume that Python exceptions can originate from within this function.

Without the except -1:

cdef inline bint c_is_list_like(object obj, bint allow_sets): raise ZeroDivisionError() return ( isinstance(obj, abc.Iterable) # we do not count strings/unicode/bytes as list-like and not isinstance(obj, (str, bytes)) # exclude zero-dimensional numpy arrays, effectively scalars and not (util.is_array(obj) and obj.ndim == 0) # exclude sets if allow_sets is False and not (allow_sets is False and isinstance(obj, abc.Set)) )

>>> import pandas as pd >>> pd.api.types.is_list_like([]) ZeroDivisionError Exception ignored in: 'pandas._libs.lib.c_is_list_like' ZeroDivisionError: False

With the except -1:

cdef inline bint c_is_list_like(object obj, bint allow_sets) except -1: raise ZeroDivisionError() return ( isinstance(obj, abc.Iterable) # we do not count strings/unicode/bytes as list-like and not isinstance(obj, (str, bytes)) # exclude zero-dimensional numpy arrays, effectively scalars and not (util.is_array(obj) and obj.ndim == 0) # exclude sets if allow_sets is False and not (allow_sets is False and isinstance(obj, abc.Set)) )

>>> import pandas as pd >>>pd.api.types.is_list_like([]) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "pandas/_libs/lib.pyx", line 985, in pandas._libs.lib.is_list_like return c_is_list_like(obj, allow_sets) File "pandas/_libs/lib.pyx", line 989, in pandas._libs.lib.c_is_list_like raise ZeroDivisionError() ZeroDivisionError

(-1 seems to be a valid value for bint)

The ignored exception led to a segfault

Any idea what value was being passed? Its hard to imagine what could cause this to raise unless you specifically rigged it to make isinstance checks fail

Aha - so after digging up some old code, it looks the source of the problem was a RecursionError, which happened to be thrown within isinstance here.

Minimal repro (segfaults for me):

In [1]: import pandas as pd In [2]: def foo(): ...: pd.api.types.is_list_like("test") ...: foo()

-1 is appropriate here as it signals cython that there maybe is an exception to check.

jreback

if u want to add the test ok or can just push this

jreback · 2020-04-23T01:14:14Z

doc/source/whatsnew/v1.1.0.rst

@@ -659,6 +659,8 @@ Other
 - Bug in :meth:`Series.map` not raising on invalid ``na_action`` (:issue:`32815`)
 - Bug in :meth:`DataFrame.__dir__` caused a segfault when using unicode surrogates in a column name (:issue:`25509`)
 - Bug in :meth:`DataFrame.plot.scatter` caused an error when plotting variable marker sizes (:issue:`32904`)
+- Propagate Python exceptions from :func:`_lib.c_is_list_like` (#33721)


don’t need the whats new as this is pretty obscure and not user facing

WillAyd

I still don't think this is the right solution. As is this function would never return -1 so it's a little confusing to declare that a return value of -1 should propogate errors.

Do you know within this function where the segfault actually happens? I think the issue is probably with util.is_array if anything

WillAyd · 2020-04-23T01:36:16Z

May also be an issue that is_array is defined in pandas/_libs/tslibs/util.pxd but this seems to try and access from pandas/_libs/util.pxd

jbrockmendel · 2020-04-23T02:26:39Z

May also be an issue that is_array is defined in pandas/_libs/tslibs/util.pxd but this seems to try and access from pandas/_libs/util.pxd

Out of skepticism I tried changing the cimport and it didnt change the segfault.

shwina · 2020-04-23T10:34:37Z

I still don't think this is the right solution. As is this function would never return -1 so it's a little confusing to declare that a return value of -1 should propogate errors.

The fact that the function will never return -1 is exactly why it is used to signal exceptions. Here are the relevant Cython docs:

https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#error-return-values

This pattern is also used in other places in lib.pyx:

https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/lib.pyx#L1096

shwina · 2020-04-23T10:58:10Z

Do you know within this function where the segfault actually happens? I think the issue is probably with util.is_array if anything

In this particular case, a segfault happens because a RecursionError is thrown from within the function (because the stack happens to be exhausted there), but is ignored.

jbrockmendel · 2020-04-23T16:27:48Z

The whatsnew can be removed, then LGTM

jreback · 2020-04-23T17:10:10Z

pandas/_libs/lib.pxd

@@ -1 +1 @@
-cdef bint c_is_list_like(object, bint)
+cdef bint c_is_list_like(object, bint) except -1


-1 is appropriate here as it signals cython that there maybe is an exception to check.

WillAyd · 2020-04-23T18:29:49Z

Misunderstanding on my end - thanks for the link. Lgtm

…

Sent from my iPhone

On Apr 23, 2020, at 10:10 AM, Jeff Reback ***@***.***> wrote: @jreback requested changes on this pull request. In pandas/_libs/lib.pxd: > @@ -1 +1 @@ -cdef bint c_is_list_like(object, bint) +cdef bint c_is_list_like(object, bint) except -1 -1 is appropriate here as it signals cython that there maybe is an exception to check. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

jreback

@shwina if you can add the test as proposed would be great (the recursion one); ping on green.

shwina · 2020-04-23T21:28:56Z

Thanks -- will do. Any suggestions for where to put that test?

jreback · 2020-04-23T21:32:12Z

Thanks -- will do. Any suggestions for where to put that test?

pandas/tests/dtypes/test_inference.py

shwina · 2020-04-24T12:22:10Z

I'm now having second thoughts about the robustness of this test: it assumes that the RecursionError will be raised from within c_is_list_like, but it could as well be raised from anywhere else within is_list_like(). Granted, for now, is_list_like() just calls c_is_list_like, but that could conceivably change in the future.

Any thoughts here?

jbrockmendel · 2020-04-24T14:31:31Z

Any thoughts here?

what if got rid of c_is_list_like and just made is_list_like cpdef?

shwina · 2020-04-24T14:51:22Z

That would solve this problem. But it looks like c_is_list_like is used elsewhere within the Cython (e.g., https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/reshape.pyx#L124).

Not sure whether the overhead that will be introduced by calling a cpdef method instead is something to worry about.

shwina · 2020-04-24T14:54:34Z

I think that having a fast cdef method is valuable here.

WillAyd · 2020-04-24T15:30:09Z

@shwina can you post the test that you have for now? Will be easier for review and filing comments

jreback · 2020-04-27T00:36:24Z

lgtm can u merge master and resolve conflicts and ping in green

shwina · 2020-04-27T13:48:44Z

Thanks @jreback - done.

jreback · 2020-04-27T18:58:10Z

thanks @shwina

… (pandas-dev#33723)

BUG: Propagate Python exceptions from c_is_list_like (pandas-dev#33721)

da1f74a

WillAyd requested changes Apr 22, 2020

View reviewed changes

shwina mentioned this pull request Apr 22, 2020

BUG: Exceptions not propagated from c_is_list_like in lib.pyx #33721

Closed

jreback approved these changes Apr 23, 2020

View reviewed changes

WillAyd requested changes Apr 23, 2020

View reviewed changes

Remove whatsnew entry

a773ba5

jreback requested changes Apr 23, 2020

View reviewed changes

jreback added the Bug label Apr 23, 2020

jreback added this to the 1.1 milestone Apr 23, 2020

jreback reviewed Apr 23, 2020

View reviewed changes

Add test for recursion error thrown from is_list_like()

25a0afe

jreback added the Segfault Non-Recoverable Error label Apr 27, 2020

jreback approved these changes Apr 27, 2020

View reviewed changes

Merge branch 'master' into propagate-py-exception-lib-pyx

50812d8

jreback merged commit b828d74 into pandas-dev:master Apr 27, 2020

rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020

BUG: Propagate Python exceptions from c_is_list_like (pandas-dev#33721)…

b92cc92

… (pandas-dev#33723)

shwina mentioned this pull request Jun 29, 2020

[REVIEW] Add DeviceBuffer.release() cdef method rapidsai/rmm#427

Merged

shwina mentioned this pull request Jul 22, 2020

[REVIEW] introduced kafka datasource to python rapidsai/cudf#5618

Merged

shwina mentioned this pull request Oct 23, 2020

Add stream wrapper types rapidsai/rmm#608

Merged

		@@ -1 +1 @@
		cdef bint c_is_list_like(object, bint)
		cdef bint c_is_list_like(object, bint) except -1

Uh oh!

BUG: Propagate Python exceptions from c_is_list_like (#33721) #33723

BUG: Propagate Python exceptions from c_is_list_like (#33721) #33723

Uh oh!

Conversation

shwina commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WillAyd Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

shwina Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrockmendel Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

shwina Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

shwina Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jreback Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

jreback Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Apr 23, 2020

Uh oh!

jbrockmendel commented Apr 23, 2020

Uh oh!

shwina commented Apr 23, 2020

Uh oh!

shwina commented Apr 23, 2020

Uh oh!

jbrockmendel commented Apr 23, 2020

Uh oh!

jreback Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

WillAyd commented Apr 23, 2020 via email

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

shwina commented Apr 23, 2020

Uh oh!

jreback commented Apr 23, 2020

Uh oh!

shwina commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbrockmendel commented Apr 24, 2020

Uh oh!

shwina commented Apr 24, 2020

Uh oh!

shwina commented Apr 24, 2020

Uh oh!

WillAyd commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback commented Apr 27, 2020

Uh oh!

shwina commented Apr 27, 2020

Uh oh!

jreback commented Apr 27, 2020

Uh oh!

Uh oh!

shwina commented Apr 22, 2020 •

edited

Loading

shwina Apr 22, 2020 •

edited

Loading

shwina Apr 22, 2020 •

edited

Loading

shwina commented Apr 24, 2020 •

edited

Loading

WillAyd commented Apr 24, 2020 •

edited

Loading